warn when neither reader nor admin roles are selected for a user #3337
Conversation
disallow saving without at least one of these roles present (because lots of pages require at least the reader permissions) fixes #2116
There was a problem hiding this comment.
Apart from the mentioned issues, this works well in the tests I did on the edit user page. Unfortunately, it is still possible to not include the reader/admin roles in the create user form.
As the Input containing both usages of RolesSelect is basically the same, I would suggest adding a new component that takes care of displaying the Input and validating the roles as these changes do.
| render() { | ||
| const user = this.props.user; | ||
| if (!this.state.roles) { | ||
| return <Spinner />; | ||
| } | ||
| let rolesAlert = null; | ||
| const roles = this.state.newRoles; | ||
| if (roles != null && roles.indexOf('Reader') === -1 && roles.indexOf('Admin') === -1) { |
There was a problem hiding this comment.
You could replace roles.indexOf() with roles.includes() for better reading experience. The polyfill we use should take care of converting it if the browser does not support it.
There was a problem hiding this comment.
Nice, didn't know about it!
| let rolesAlert = null; | ||
| const roles = this.state.newRoles; | ||
| if (roles != null && roles.indexOf('Reader') === -1 && roles.indexOf('Admin') === -1) { | ||
| rolesAlert = <Alert bsStyle="danger" role="alert" className={EditRolesFormStyle.rolesMissingAlert}> |
There was a problem hiding this comment.
Small nitpick: this triggers a react/wrap-multilines styling warning.
switch to using Array#includes for clarity
|
I opted for a simpler approach by duplication the few lines of code, rather than passing around styling props and the validation state. |
disallow saving without at least one of these roles present (because lots of pages require at least the reader permissions)
fixes #2116